add opt-in responses background mode guardrails#317
add opt-in responses background mode guardrails#317ndycode wants to merge 4 commits intofeat/openai-parity-pr8from
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughadds a new opt-in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Config as Config/Plugin
participant Validator as Request Transformer
participant Codex as Codex Backend
Client->>Config: request with background: true
Config->>Config: check allowBackgroundResponses
alt background disabled
Config->>Validator: allowBackgroundResponses = false
Validator->>Validator: detect background: true
Validator->>Validator: throw "Responses background mode not enabled"
Validator-->>Client: reject
else background enabled
Config->>Validator: allowBackgroundResponses = true
Validator->>Validator: validate store !== false
alt store: false detected
Validator->>Validator: throw "background mode requires store=true"
Validator-->>Client: reject
else valid
Validator->>Validator: force store=true
Validator->>Validator: preserve input IDs
Validator->>Validator: skip fast-session trimming
Validator->>Codex: send transformed request
Codex-->>Client: background response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes specific concerns to flag:missing test coverage:
windows edge cases:
concurrency risks:
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ndycode
added
the
needs-human-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/development/CONFIG_FIELDS.md (1)
192-206:⚠️ Potential issue | 🟡 Minordocument the missing environment variable override for
backgroundResponses.the env var
CODEX_AUTH_BACKGROUND_RESPONSES(mentioned in context snippets fromlib/request/request-transformer.ts:237-254) is not listed in the environment overrides section. add an entry here so users know they can toggle background mode via environment variables.as per coding guidelines: "keep README, SECURITY, and docs consistent with actual CLI flags and workflows."
📝 proposed addition to environment overrides table
| `CODEX_MULTI_AUTH_REAL_CODEX_BIN` | Force official Codex binary path | | `CODEX_MULTI_AUTH_BYPASS` | Bypass local auth handling | +| `CODEX_AUTH_BACKGROUND_RESPONSES` | Enable responses background mode |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/development/CONFIG_FIELDS.md` around lines 192 - 206, Add a new row to the environment overrides table documenting the CODEX_AUTH_BACKGROUND_RESPONSES variable: include the exact env var name `CODEX_AUTH_BACKGROUND_RESPONSES`, a short purpose like "Toggle backgroundResponses mode for request handling", and place it near the other CODEX_AUTH_* entries so it matches the implementation referenced by the backgroundResponses symbol in request-transformer (the code that reads CODEX_AUTH_BACKGROUND_RESPONSES). Ensure the markdown table row follows the same formatting as the other entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development/CONFIG_FIELDS.md`:
- Around line 84-85: Add upgrade notes for the new backgroundResponses
compatibility switch in the CONFIG_FIELDS.md entry for backgroundResponses:
document when to enable the flag, clearly explain the behavioral difference
between stateful routing (store=true) and stateless routing (store=false), and
list any breaking changes or migration steps (e.g., how existing stateless
pipelines will be affected, required configuration changes, and recommended
testing steps). Keep the notes concise, include examples of expected effects on
Requests API `background: true` behavior, and mark this as required for any
release-targeted PR so upgrade guidance is present for operators.
In `@docs/reference/public-api.md`:
- Around line 71-75: Add explicit upgrade-note and top-level docs updates to
document the new backgroundResponses toggle and the
CODEX_AUTH_BACKGROUND_RESPONSES env flag: update the upgrade guide and
README/SECURITY docs to describe the new behavior (how
backgroundResponses/CODEX_AUTH_BACKGROUND_RESPONSES enable stateful background
mode, force store=true, preserve prompt_cache_retention and text.format, and
implications for caller-supplied input item IDs), include rollout guidance and
any required CLI/ npm script changes, and reference the config toggle symbol
backgroundResponses (and env var CODEX_AUTH_BACKGROUND_RESPONSES) so readers can
find the implementation (see lib config symbol backgroundResponses and provider
option promptCacheRetention) and ensure docs and upgrade notes are consistent
with the code changes.
In `@test/plugin-config.test.ts`:
- Around line 692-709: The tests in getBackgroundResponses mutate the
CODEX_AUTH_BACKGROUND_RESPONSES env var without restoring it, causing
order-dependent failures; update the describe('getBackgroundResponses') block to
save the original process.env.CODEX_AUTH_BACKGROUND_RESPONSES before tests and
restore it after (or ensure deletion) using vitest hooks (beforeEach/afterEach
or afterAll), so getBackgroundResponses tests leave process.env exactly as they
found it and do not leak into other tests.
In `@test/request-transformer.test.ts`:
- Around line 2517-2538: Add a test that mirrors the existing
background-mode/store=false assertion but targets the providerOptions path: call
transformRequestBody with background: true and providerOptions: { openai: {
store: false } } (instead of top-level store: false) and assert it rejects with
the same error about "Responses background mode requires store=true and cannot
be combined with stateless store=false routing."; this ensures
transformRequestBody's providerOptions.openai.store handling is covered and
prevents the regression where only top-level store is validated.
---
Outside diff comments:
In `@docs/development/CONFIG_FIELDS.md`:
- Around line 192-206: Add a new row to the environment overrides table
documenting the CODEX_AUTH_BACKGROUND_RESPONSES variable: include the exact env
var name `CODEX_AUTH_BACKGROUND_RESPONSES`, a short purpose like "Toggle
backgroundResponses mode for request handling", and place it near the other
CODEX_AUTH_* entries so it matches the implementation referenced by the
backgroundResponses symbol in request-transformer (the code that reads
CODEX_AUTH_BACKGROUND_RESPONSES). Ensure the markdown table row follows the same
formatting as the other entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b0e4b4e-b1ba-424f-a537-308a2eddbcca
📒 Files selected for processing (13)
docs/development/CONFIG_FIELDS.mddocs/reference/public-api.mdindex.tslib/config.tslib/request/fetch-helpers.tslib/request/request-transformer.tslib/schemas.tslib/types.tstest/fetch-helpers.test.tstest/index.test.tstest/plugin-config.test.tstest/public-api-contract.test.tstest/request-transformer.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (3)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/types.tslib/schemas.tslib/config.tslib/request/fetch-helpers.tslib/request/request-transformer.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/development/CONFIG_FIELDS.mddocs/reference/public-api.md
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/public-api-contract.test.tstest/plugin-config.test.tstest/index.test.tstest/fetch-helpers.test.tstest/request-transformer.test.ts
🔇 Additional comments (6)
lib/types.ts (1)
192-192: lgtm!the optional
backgroundfield is correctly typed and integrates cleanly with the existingRequestBodyinterface. backward compatible since it's optional.test/index.test.ts (1)
103-103: add integration tests for background mode in this file.the mock at line 103 stubs
getBackgroundResponses, but test/index.test.ts has zero tests exercising background mode request handling. the feature is implemented across lib/request/request-transformer.ts:234, 246, 251, 928 and lib/request/fetch-helpers.ts:746, with regression tests present in test/fetch-helpers.test.ts:1138+ and test/request-transformer.test.ts:2477+. however, index.test.ts (the integration test layer) never exercises:
- requests with
body.background = truewhengetBackgroundResponsesreturnstrue- the error path at lib/request/request-transformer.ts:246 when background is disabled
- store transformation at lib/request/request-transformer.ts:928 through the full handler stack
- id stripping behavior differences between background and non-background requests
add integration tests covering these flows to ensure the fetch handler correctly propagates background mode through the plugin.
index.ts (1)
1364-1376: good gating wire-up for background responses.this passes the new opt-in flag directly into request transformation and keeps the default path unchanged. this matches
lib/config.ts:949and the contract checks intest/public-api-contract.test.ts:157.test/public-api-contract.test.ts (1)
116-165: contract coverage update looks solid.the test now explicitly protects positional/named parity for background-mode requests and asserts
background/storepreservation under opt-in (test/public-api-contract.test.ts:146,test/public-api-contract.test.ts:164).lib/schemas.ts (1)
51-51: schema addition is safe and backward compatible.making
backgroundResponsesoptional preserves existing config files while validating the new flag path (lib/schemas.ts:51,lib/config.ts:152).lib/config.ts (1)
949-955: getter implementation is consistent with existing config precedence.the new accessor correctly applies env override → config → default ordering, aligned with the rest of
lib/config.tsand covered intest/plugin-config.test.ts:692.
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
ndycode
force-pushed
the
feat/openai-parity-pr9
branch
from
21b6720 to
66f5d33
Compare
ndycode
removed
the
needs-human-review
ndycode
dismissed
coderabbitai[bot]’s stale review
All review threads are resolved and later commits addressed this stale automated change request.
1 participant
Summary
Stack
#316